-
Notifications
You must be signed in to change notification settings - Fork 38.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce internal channels for scheduler #84336
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/remove-kind feature
/kind cleanup
/lgtm
/lgtm cancel |
1a50b5f
to
e037f0f
Compare
e037f0f
to
c5f651c
Compare
c5f651c
to
5cb6d90
Compare
/test pull-kubernetes-kubemark-e2e-gce-big |
can you please rebase and resolve the conflicts? |
8eef080
to
a6c2b11
Compare
I just rebased. Had to upgrade go to 1.13 to do so. But now it seems some of the scripts in the |
/retest |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g, deads2k, hprateek43 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Any specific errors? This can't merge otherwise. |
I had a discussion with @liggitt around this. I just setup my source code inside GOPATH, but am still struggling to get the bazel update running. Will update this in a day or two. Working on that. |
a6c2b11
to
db97001
Compare
@alculquicondor Seems sorted for now. Should be clear for merge? |
@@ -261,6 +261,8 @@ | |||
"k8s.io/kubernetes/pkg/capabilities", | |||
"k8s.io/kubernetes/pkg/master/ports", | |||
"k8s.io/kubernetes/pkg/scheduler/api", | |||
"k8s.io/kubernetes/pkg/scheduler/internal", | |||
"k8s.io/kubernetes/pkg/scheduler/internal/errchannel", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't want controllers depending on internal scheduler packages, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually we dont. I plan to remove this coupling in the coming weeks, the code can be refactored to remove this dependency. This is added for now for the scheduler cleanup. Can be marked as a TODO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is intended to be a cleanup PR, it would be preferable not to introduce new debt in it. can that dependency be dropped before we mark this internal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hprateek43, how do you plan to remove the dependency?
Other than that, I don't understand why import-restrictions apply to indirect imports. I feel like it shouldn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the idea is to first eliminate the utils package and then refactor predicates to remove dependencies on internal packages. But @alculquicondor is also right in his place that transient dependencies should not be considered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just as a reminder, .import-restrictions
is a home-grown k8s utility that requires us to whitelist all transitive dependencies. The go compiler prevents pkg/controller
from directly importing anything in pkg/scheduler/internal/...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liggitt This point could be considered. Although now I am planning to remove the error channel altogether
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liggitt This point could be considered
that's a good point, and would remove my objection
Although now I am planning to remove the error channel altogether
that sounds even better :)
2a41943
to
7521123
Compare
/test pull-kubernetes-node-e2e-containerd |
1 similar comment
/test pull-kubernetes-node-e2e-containerd |
@liggitt can you give a |
/hold |
@misterikkit Can you share your input here? We had a discussion over transitive dependencies over https://github.com/kubernetes/kubernetes/pull/76990/files where you stated that this is more of a quirk of the tool? Do you think this change seems logically correct or we should probably refactor it in a better way ? |
7521123
to
980907d
Compare
Closing this PR as consensus is not formed regarding internal channels |
Sorry I just noticed this. My opinion is that the At its core, it is just a wrapper for non-blocking reads and writes on a channel. Adding some select statements to the codebase is not horrible, and if we don't like doing that, having much smaller helper functions just to wrap a The part where we pass a cancel function into |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR introduces internal channels for scheduler with the general idea of phasing out the scheduler
util
package. The error channels for scheduler must be internal and should not be accessible to other components.Which issue(s) this PR fixes:
Fixes #84338
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: